Flaky tests detected in c26f863.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6191995244
📝 Reported issues:

• [Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js

Size Change: -937 B (0%)

Total Size: 1.62 MB

_{compressed-size-action}

@tyxla @ciampo - I've updated some of the failing tests to include an expect warning assertion. There are two more failing tests which still need to be updated but:

1. I wanted to ensure the first few tests I've updated so far is the expected way to handle adding the deprecation warning.
2. The failing tests are both test.each cases. The first set of conditions expects a warning, and the other doesn't. For packages/components/src/font-size-picker/test/index.tsx, we could easily separate the test.each into single tests.

diff --git a/packages/components/src/font-size-picker/test/index.tsx b/packages/components/src/font-size-picker/test/index.tsx
index 0d7816399b..b83a4a11be 100644
--- a/packages/components/src/font-size-picker/test/index.tsx
+++ b/packages/components/src/font-size-picker/test/index.tsx
@@ -36,32 +36,48 @@ describe( 'FontSizePicker', () => {
 		}
 	);
 
-	test.each( [
-		// Use units when first font size uses units.
-		{ firstFontSize: '12px', expectedValue: '80px' },
-		// Don't use units when first font size does not use units.
-		{ firstFontSize: 12, expectedValue: 80 },
-	] )(
-		'should call onChange( $expectedValue ) after user types 80 when first font size is $firstFontSize',
-		async ( { firstFontSize, expectedValue } ) => {
-			const user = userEvent.setup();
-			const onChange = jest.fn();
-			render(
-				<FontSizePicker
-					__nextHasNoMarginBottom
-					fontSizes={ [ { slug: 'slug', size: firstFontSize } ] }
-					onChange={ onChange }
-				/>
-			);
-			await user.click(
-				screen.getByRole( 'button', { name: 'Set custom size' } )
-			);
-			const input = screen.getByLabelText( 'Custom' );
-			await user.type( input, '80' );
-			expect( onChange ).toHaveBeenCalledTimes( 2 ); // Once per keystroke.
-			expect( onChange ).toHaveBeenCalledWith( expectedValue );
-		}
-	);
+	test( 'should call onChange( 80px ) after user types 80 when first font size is 12px', async () => {
+		const user = userEvent.setup();
+		const onChange = jest.fn();
+		render(
+			<FontSizePicker
+				__nextHasNoMarginBottom
+				fontSizes={ [ { slug: 'slug', size: '12px' } ] }
+				onChange={ onChange }
+			/>
+		);
+		await user.click(
+			screen.getByRole( 'button', { name: 'Set custom size' } )
+		);
+		const input = screen.getByLabelText( 'Custom' );
+
+		await user.type( input, '80' );
+
+		expect( console ).toHaveWarned();
+		expect( onChange ).toHaveBeenCalledTimes( 2 ); // Once per keystroke.
+		expect( onChange ).toHaveBeenCalledWith( '80px' );
+	} );
+
+	test( 'should call onChange( 80 ) after user types 80 when first font size is 12', async () => {
+		const user = userEvent.setup();
+		const onChange = jest.fn();
+		render(
+			<FontSizePicker
+				__nextHasNoMarginBottom
+				fontSizes={ [ { slug: 'slug', size: 12 } ] }
+				onChange={ onChange }
+			/>
+		);
+		await user.click(
+			screen.getByRole( 'button', { name: 'Set custom size' } )
+		);
+		const input = screen.getByLabelText( 'Custom' );
+
+		await user.type( input, '80' );
+
+		expect( onChange ).toHaveBeenCalledTimes( 2 ); // Once per keystroke.
+		expect( onChange ).toHaveBeenCalledWith( 80 );
+	} );
 
 	describe( 'with > 5 homogeneous font sizes', () => {
 		const fontSizes = [

But in packages/components/src/toggle-group-control/test/index.tsx, it gets a bit messier. The easiest solution would be to add a conditional statement, but I saw it's not good practice to put expects in conditionals. I have seen a handful of tests in GB with an ignore // eslint-disable-next-line jest/no-conditional-expect, but I wanted to check to see if there were any other approaches I've missed. Although it would be as simple as adding the following (with a different condition for FontSizePicker` of course):

diff --git a/packages/components/src/toggle-group-control/test/index.tsx b/packages/components/src/toggle-group-control/test/index.tsx
index 78be0e89fc..d685833628 100644
--- a/packages/components/src/toggle-group-control/test/index.tsx
+++ b/packages/components/src/toggle-group-control/test/index.tsx
@@ -98,6 +98,11 @@ describe.each( [
 				</Component>
 			);
 
+			if ( mode === 'uncontrolled' ) {
+				// eslint-disable-next-line jest/no-conditional-expect
+				expect( console ).toHaveWarned();
+			}
+
 			expect( container ).toMatchSnapshot();
 		} );
 	} );

This is what i'd do:

• I would update, in this PR, all consumers of Tooltip to use the placement prop instead of position:
  • when possible, we simply update the hardcoded values (for example, position="top center" becomes placement="top"
  • if, instead, the consumer is forwarding a prop (like Button, that forwards the value of tooltipPosition), then we can use the positionToPlacement utility function, and do something like placement={positionToPlacement(tooltipPosition)}
• One we're done refactoring the consumers of Tooltip, the only tests that would need the expect( console ).toHaveWarned(); assertion should be the Tooltip tests (which should also avoid the issues flagged in the comment above)
• As a follow-up to this PR, we can update those unit tests to also use the placement prop, and thus remove the expect( console ).toHaveWarned(); assertions

Thanks for the comments! I was hoping to replace internal usage in a second PR, but it seems cleaner to do it in this PR. That way we avoid adding warnings and rewriting tests temporarily to then be reverted in the follow-up PR.

@ciampo I noticed in similar instances where we added placement, we didn't add a CHANGELOG entry. I would have thought it would be under internal, but I'll wait on you to confirm which way to go.

@ciampo I noticed in similar instances where we added placement, we didn't add a CHANGELOG entry.

The specific example of URLPopover didn't require a CHANGELOG entry because that component is not part of the @wordpress/components package

I would have thought it would be under internal, but I'll wait on you to confirm which way to go.

Since we're adding a new public API to the Tooltip component, I'd argue that it not an internal change — I'd probably list it under a ### New Features section.

Since we're adding a new public API to the Tooltip component, I'd argue that it not an internal change — I'd probably list it under a ### New Features section.

@ciampo do you think it would make sense for hideOnClick to be under ### New Features as well? I could move it when updating the changelog here

Noting that something went off with the latest merge from trunk - there seem to be some extra changes that we likely don't want as part of the PR.

do you think it would make sense for hideOnClick to be under ### New Features as well? I could move it when updating the changelog here

Yup, sounds good to me.

Noting that something went off with the latest merge from trunk - there seem to be some extra changes that we likely don't want as part of the PR.

Good catch. Let's fix that before merging.

Noting that something went off with the latest merge from trunk - there seem to be some extra changes that we likely don't want as part of the PR.

Great find! Thank you for looking out. 😄 I am trying to be friends with git merge over git rebase, but... 😕

I'll get that resolved along with the conflict and update the changelog.